Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for per-namespace configuration and add setting for workspace PVC request size #487

Merged
merged 3 commits into from
Jul 16, 2021
Merged

Add support for per-namespace configuration and add setting for workspace PVC request size #487

merged 3 commits into from
Jul 16, 2021

Conversation

amisevsk
Copy link
Collaborator

@amisevsk amisevsk commented Jul 10, 2021 •
edited
Loading

What does this PR do?

  • Add mechanism for defining namespace-level configuration for DevWorkspaces via configmap
    • A configmap is marked as relevant with the label controller.devfile.io/namespaced-config
  • Add capability to define PVC size via configmap above.

TODOs: (either here or separate issues)

  • Workspace PVCs are not updated once they are created, so the setting above only applies when the claim-devworkspace PVC does not exist. Workspace PVCs are never updated and are not removed from the cluster #488
    • It's not straightforward to enable this, as only the PVC storage request can be changed after creation. The SyncObjects method won't work if it uses Update() and switching to Patch() fails for reasons I don't fully understand yet (likely due to dealing with runtime.Object)
  • The configuration setting currently applies to the PVC's storage request and no limit is supplied. Should we allow the configmap to define both request and limit?

What issues does this PR fix or reference?

Closes #477

Is it tested? How?

Create a configmap to set defaults for a specific namespace:

cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: ConfigMap
metadata:
  name: test-nsconfig
  labels:
    controller.devfile.io/namespaced-config: "true"
data:
  commonPVCSize: 5Gi
EOF

and then create a DevWorkspace in that namespace.

If the namespace already has a claim-devworkspace, it will not be updated and so should be deleted before testing.

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path to trigger)
    • v7-devworkspaces-operator-e2e: DevWorkspace e2e test
    • v7-devworkspace-happy-path: DevWorkspace e2e test

@openshift-ci openshift-ci bot requested a review from JPinkney July 10, 2021 00:36
sleshchenko
sleshchenko reviewed Jul 14, 2021
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM
will test soon

sleshchenko
sleshchenko reviewed Jul 15, 2021
View reviewed changes
pkg/provision/config/config.go
)

const (
commonPVCSizeKey = "commonPVCSize"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, during testing I found more config we may want to configure, but I'm not sure.
Like pvc name. For CRW it's claim-che-workspace, not sure if CRW expects Che Workspace and DevWorkspaces uses different PVCs.

Also, see che.infra.kubernetes.pvc.storage_class_name.

it's out of the current PR scope for sure, but WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that the config struct could be extended to include additional information as needed (storage class makes sense).

For common PVC name, I think it's a limited use case that would be complex to support -- e.g. what happens if it is updated after claim-che-workspace has been created? It seems like it would be an open door for leaking storage after workspaces are deleted, etc.

sleshchenko
sleshchenko approved these changes Jul 15, 2021
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works fine

@openshift-ci openshift-ci bot added the lgtm label Jul 15, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, sleshchenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@amisevsk
Copy link
Collaborator Author

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@openshift-ci openshift-ci bot removed the lgtm label Jul 15, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 15, 2021

New changes are detected. LGTM label has been removed.

@amisevsk
Copy link
Collaborator Author

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@amisevsk
Add support for per-namespace configuration
29de48b 
Add support for configuring namespace-wide settings for DevWorkspaces by
created a configmap with the appropriate label.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Account for namespaced configuration for common PVC size.
a331312 
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Fix tests to accomodate customizable common PVC size
43e2590 
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Copy link
Collaborator Author

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@amisevsk
Copy link
Collaborator Author

/test v7-devworkspaces-operator-e2e

@amisevsk amisevsk merged commit c75f07d into devfile:main Jul 16, 2021
@amisevsk amisevsk deleted the per-workspace-config branch July 16, 2021 16:53
@sleshchenko sleshchenko mentioned this pull request Sep 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sleshchenko sleshchenko sleshchenko approved these changes

@JPinkney JPinkney Awaiting requested review from JPinkney

Assignees

@sleshchenko sleshchenko

Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an ability to configure common PVC size on namespace level
2 participants
@amisevsk @sleshchenko